Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

connectivity: Ignore expected XFRM errors #2190

Merged
merged 2 commits into from
Dec 18, 2023

Conversation

pchaigno
Copy link
Member

@pchaigno pchaigno commented Dec 15, 2023

First commit refactors the existing logic for the --expected-drop-reasons. The second adds a new --expected-xfrm-errors.

This commit refactors computeExpectedDropReasons to move the core logic
to the utils package. We will need to reuse that logic for another set
of exceptions, this time for XFRM errors.

There are no functional changes in this commit.

Signed-off-by: Paul Chaignon <[email protected]>
We have a couple XFRM errors, XfrmInError and XfrmFwdHdrError, that can
be expected to slightly increase in specific scenarios. Those have
started occuring in CI so we need a way to ignore them.

This commit implements that, as well as a connectivity test flag to
control the list of XFRM errors to ignore. This is similar to what I did
for drop reasons in commit 4880c91 ("connectivity: Check for
unexpected packet drops") and b5e40ba ("connectivity: Add flag
--expected-drop-reasons").

In addition, because those errors are only expected to happen in very
small amount, we only ignore them if there are less than 10 such errors.
It is critical that we fail tests if these errors become more frequent.

Signed-off-by: Paul Chaignon <[email protected]>
@pchaigno pchaigno force-pushed the pr/pchaigno/expected-xfrm-errors branch from 6974211 to 7e3a3e4 Compare December 15, 2023 17:49
@pchaigno pchaigno marked this pull request as ready for review December 15, 2023 23:35
@pchaigno pchaigno requested review from a team as code owners December 15, 2023 23:35
@pchaigno pchaigno requested review from asauber, jibi, christarazi, nbusseneau and michi-covalent and removed request for christarazi and asauber December 15, 2023 23:35
Copy link
Member

@jschwinger233 jschwinger233 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If len(inputExceptions) == 0, ComputeFailureExceptions returns an empty slice regardless of defaultExceptions. But the original logic appears to be same, there should be not a problem.

@pchaigno
Copy link
Member Author

If len(inputExceptions) == 0, ComputeFailureExceptions returns an empty slice regardless of defaultExceptions. But the original logic appears to be same, there should be not a problem.

Yeah, that bugged me a bit as well. The flag takes defaults.ExpectedXFRMErrors as the default value, so if no flag is given, len(inputExceptions) is non zero and we end up just using the default exceptions. The case len(inputExceptions) == 0 would be if we pass an empty list of exceptions via the flag; not sure if that's useful, but it would then end up not using any exception (not even the defaults).

I believe that should match the intended behavior from users of the flag.

@pchaigno pchaigno merged commit b62d4da into main Dec 18, 2023
13 checks passed
@pchaigno pchaigno deleted the pr/pchaigno/expected-xfrm-errors branch December 18, 2023 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants